fix: register Value subclass auto-methods in class hierarchy before type checking#1018
Merged
fix: register Value subclass auto-methods in class hierarchy before type checking#1018
Conversation
…ype checking The type checker ran before codegen, so auto-generated keyword constructors (e.g., `x:y:`), getters, and `with*:` setters for `Value subclass:` classes were invisible to method bodies in the same class. This caused false "does not understand" errors when a method referenced its own class's keyword constructor (e.g., `translateX:y:` calling `Point x:y:`). - Add `add_value_auto_methods()` to synthesize slot methods in the class hierarchy during semantic analysis, mirroring codegen's logic - Extend `is_effect_free` lint to cover MapLiteral, ArrayLiteral, and ListLiteral (with recursive purity checks on contents) - Add unit tests for both changes Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review infoConfiguration used: Organization UI Review profile: CHILL Plan: Pro Disabled knowledge base sources:
📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds auto-generated Value-slot methods into class construction and reflection metadata; treats map/array/list literals as effect-free for linting; extends tests for map-literal linting and value auto-method generation; emits a warning with a functional-update hint for non-self field assignments. Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
…gnose non-self field assignment - generate_meta_function now includes auto-generated slot methods (getters, with*: setters, keyword constructor) in __beamtalk_meta/0 so Point methods / allMethods shows the full public interface - Type checker emits a clear diagnostic for `other.field := value` instead of letting it reach codegen as an unsupported feature error; hints the user to use `p := p withX: newValue` instead - Make AutoSlotMethods, compute_auto_slot_methods, and with_star_selector pub(super) so gen_server/methods.rs can reuse the selector logic Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/beamtalk-core/src/semantic_analysis/type_checker.rs`:
- Around line 280-314: The non-`self` receiver diagnostic is currently only
emitted when the receiver is an Expression::Identifier, so compound receivers
like `(obj foo).x := v` skip it; change the logic in type_checker.rs (around the
block using Expression::Identifier and check_field_assignment) to treat any
receiver that is not the identifier named "self" as an invalid mutation: call
self.check_field_assignment(field, &ty, *span, hierarchy, env) only when the
receiver is an Identifier whose name == "self", and in all other cases construct
the same Diagnostic::error / hint (with the with_sel message) and push it to
self.diagnostics so non-Identifier receivers also get the non-`self` assignment
diagnostic.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
crates/beamtalk-core/src/codegen/core_erlang/gen_server/methods.rscrates/beamtalk-core/src/codegen/core_erlang/value_type_codegen.rscrates/beamtalk-core/src/semantic_analysis/type_checker.rs
…gnostic Compound receivers like (obj foo).x := v previously bypassed the diagnostic and fell through to a codegen error. Now any non-self receiver (identifier or expression) correctly emits the error with a 'receiver' fallback name for the hint. Addresses CodeRabbit review comment on PR #1018. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
crates/beamtalk-core/src/semantic_analysis/type_checker.rs (1)
279-282: Consider reusing existingis_self_receiverhelper.Lines 279-282 duplicate the logic from the existing
Self::is_self_receiver()method at line 566-568. Additionally,validators.rshas a more robust version that handles parenthesized expressions like(self).field := value.Option 1: Reuse existing method
- let is_self_receiver = matches!( - receiver.as_ref(), - Expression::Identifier(recv_id) if recv_id.name == "self" - ); + let is_self_receiver = Self::is_self_receiver(receiver);Option 2: Enhance is_self_receiver to handle parenthesized expressions (matching validators.rs)
/// Returns true if the expression is `self` (direct identifier reference). fn is_self_receiver(expr: &Expression) -> bool { - matches!(expr, Expression::Identifier(ident) if ident.name == "self") + match expr { + Expression::Identifier(ident) => ident.name == "self", + Expression::Parenthesized { expression, .. } => Self::is_self_receiver(expression), + _ => false, + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/beamtalk-core/src/semantic_analysis/type_checker.rs` around lines 279 - 282, Replace the duplicated local match that computes is_self_receiver in the TypeChecker code with a call to the existing helper Self::is_self_receiver(receiver.as_ref()); if the helper doesn't yet handle parenthesized expressions (e.g., Expression::Paren), update Self::is_self_receiver to mirror the robust logic in validators.rs so it treats `(self)` the same as `self`, then use that helper from type checking (replace the local match in the TypeChecker with the helper call to avoid duplication).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/beamtalk-core/src/semantic_analysis/type_checker.rs`:
- Around line 307-312: The diagnostic in type_checker.rs uses Diagnostic::error
when reporting "Cannot assign to `{recv_name}.{field_name}` ..." which violates
the module rule "Warnings only, never errors"; change the call to
Diagnostic::warning so the type checker emits a warning instead of an error,
preserving the same formatted message and *span variable and keeping any
attached notes or labels around that Diagnostic construction in the same place
(look for the block that builds `diag` using `recv_name`, `field_name`, and
`*span` in the `type_checker.rs` file).
---
Nitpick comments:
In `@crates/beamtalk-core/src/semantic_analysis/type_checker.rs`:
- Around line 279-282: Replace the duplicated local match that computes
is_self_receiver in the TypeChecker code with a call to the existing helper
Self::is_self_receiver(receiver.as_ref()); if the helper doesn't yet handle
parenthesized expressions (e.g., Expression::Paren), update
Self::is_self_receiver to mirror the robust logic in validators.rs so it treats
`(self)` the same as `self`, then use that helper from type checking (replace
the local match in the TypeChecker with the helper call to avoid duplication).
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
crates/beamtalk-core/src/semantic_analysis/type_checker.rs
The type_checker module's design principle is "Warnings only, never errors (avoid false positives)" (module docstring lines 13-14). Changed Diagnostic::error() to Diagnostic::warning() for the non-self field assignment diagnostic to align with this principle. Added test asserting Severity::Warning (not Error) for `other.x := 1`.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Value subclass:classes with auto-generated keyword constructors (e.g.,Point x:y:) failed type checking when referenced in method bodies of the same class ("Point class does not understand 'x:y:'")with*:setters, keyword constructors) were only synthesized during codegen, but the type checker runs before codegenMapLiteral,ArrayLiteral, andListLiteralexpressions (with recursive purity checks)Key Changes
class_hierarchy/mod.rs: Addedadd_value_auto_methods()— mirrors codegen'scompute_auto_slot_methodslogicvalidators.rs:is_effect_freenow recursively checks map/array/list contents for side effectseffect_free_statement.rs: Added tests for discarded map literal lint (pure vs side-effectful)Follow-up
Test Plan
ValueSubclassTest(11 tests) continues passingexamples/getting-started/src/point.btcompiles cleanly (previously errored)🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Improvements
Tests